AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed

What was reviewed

Skipped (out of scope): dist/, pnpm-lock.yaml

⚠️ CI checks could not be fetched (command required approval). Verify checks pass before merging.

Findings

🔶 Medium — big.js listed in devDependencies but used in production code

File: packages/shared/widget-plugin-platform/package.json line 50
Problem: number-formatter.ts imports Big from big.js and calls new Big(value) in the returned formatter closure — this executes at widget runtime, not just during tests or builds. Declaring it only in devDependencies is incorrect: any consumer that builds widget-plugin-platform in isolation will fail to resolve big.js, and bundlers that trace package boundaries may emit a warning or fail. The monorepo pnpm workspace masks the issue today because the root node_modules holds the install, but the package metadata is wrong.
Fix: Move big.js from devDependencies to dependencies in packages/shared/widget-plugin-platform/package.json:

"dependencies": {
    "big.js": "^6.2.1"
},
"devDependencies": {
    // remove big.js from here
}

⚠️ Low — helpers.spec.ts in slider-web duplicates number-formatter.spec.ts in the shared package verbatim

File: packages/pluggableWidgets/slider-web/src/utils/__tests__/helpers.spec.ts
Note: Every test case (fakeNumberFormatter helper, all five createValueFormatter assertions) is an exact copy of packages/shared/widget-plugin-platform/src/utils/__tests__/number-formatter.spec.ts. The file tests a function that lives in the shared package, not in slider-web/utils/helpers. Duplicated tests diverge silently over time and add no coverage value. Consider removing helpers.spec.ts — the shared package's tests already cover createValueFormatter completely.

⚠️ Low — Range-slider handleRender is recreated on every render (inconsistent with slider-web)

File: packages/pluggableWidgets/range-slider-web/src/components/Container.tsx line 83
Note: slider-web/Container.tsx wraps createHandleRender(...) in useMemo, giving rc-slider a stable handleRender reference. The range-slider defines the function inline in JSX, so a new closure is allocated on every render. While not a correctness bug, this inconsistency means range-slider handle nodes re-mount more often than needed. Consider wrapping it in useMemo for parity:

const handleRender = useMemo(
    () => (node: ReactElement, handleProps: SliderHandleProps) => {
        const isCustomText = tooltipTypeCheck[handleProps.index] === "customText";
        const displayValue = isCustomText
            ? (tooltipValue[handleProps.index]?.value ?? "")
            : format(handleProps.value);
        return (
            <HandleTooltip
                value={displayValue}
                index={handleProps.index}
                visible={handleProps.dragging && props.showTooltip}
                sliderRef={sliderRef}
                {...props}
            >
                {node}
            </HandleTooltip>
        );
    },
    [tooltipTypeCheck, tooltipValue, format, props]
);

⚠️ Low — eslint-disable-next-line react-hooks/exhaustive-deps with no explanation

File: packages/pluggableWidgets/slider-web/src/components/Container.tsx line 36
File: packages/pluggableWidgets/range-slider-web/src/components/Container.tsx line 48
Note: Per repo conventions (AGENTS.md), suppression comments should explain the non-obvious constraint — here, why formatter cannot be used as a stable dep. A one-line note helps reviewers understand this is intentional:

// Mendix formatter is a new object reference each render; identity-compare on formatter
// would recreate the closure on every tick. We intentionally omit it and rely on
// decimalPlaces changing as the meaningful signal.
// eslint-disable-next-line react-hooks/exhaustive-deps
[props.valueAttribute.formatter, props.decimalPlaces]

Positives

• Correct root cause identified and fixed in both places: marks key uses parseFloat(rawValue.toFixed(n)) (locale-safe) while the label uses format(rawValue) — these are the right tools for each job.
• Moving createMarks and createValueFormatter to the shared package is the right call; both slider-web and range-slider-web now consume a single implementation with a single test suite.
• The useMemo wrapping of createHandleRender in slider-web Container (with correct deps including format) is a genuine improvement over the pre-PR inline creation.
• Test coverage is thorough: locale separators, trailing-zero preservation, decimalPlaces=0 regression, repeating-decimal key rounding, and all three undefined-return guard cases are all covered.
• CHANGELOG entries present in both packages in Keep a Changelog format.

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge

What was reviewed

Skipped (out of scope): dist/, pnpm-lock.yaml

Findings

⚠️ Low — big.js classified as devDependency in a runtime module

File: packages/shared/widget-plugin-platform/package.json line 39
Problem: number-formatter.ts compiles to dist/number-formatter.js with a live import { Big } from "big.js". The package is a library ("build": "tsc"; no bundling), so the import survives to dist. Listing big.js only under devDependencies is semantically wrong for a published/distributed shared package — a consumer building outside the monorepo's hoisted node_modules would fail to resolve it.

In this monorepo it works today because pnpm hoists devDeps to the root, and widget Rollup builds pick it up transitively. But it's one misconfigured install away from a silent build failure.
Fix: Move big.js to dependencies:

// package.json
"dependencies": {
    "big.js": "^6.2.1"
},
"devDependencies": {
    // remove big.js from here
}

⚠️ Low — eslint-disable-next-line react-hooks/exhaustive-deps without a why comment

File: packages/pluggableWidgets/slider-web/src/components/Container.tsx line 36, packages/pluggableWidgets/range-slider-web/src/components/Container.tsx line 47
Problem: The disable comment is on the deps array [props.valueAttribute.formatter, props.decimalPlaces]. This silences the linter without explaining whether the author is intentionally omitting props.valueAttribute from deps (to avoid recreating on every render since the formatter config, not the reference, is what matters) or something else. Per repo convention, suppression comments must explain the WHY.

Also worth a second look: if EditableValue.formatter is a new object reference on every render (common in Mendix), format will be recreated every render, which cascades to handleRender and marks. If that's acceptable (cheap to recreate), the useMemo can be removed entirely. If not, a stable key (e.g. formatter.config.decimalPrecision + formatter.config.groupDigits) should be used as the dependency.
Fix:

const format = useMemo(
    () => createValueFormatter(props.valueAttribute.formatter as NumberFormatter, props.decimalPlaces),
    // formatter is read by reference; config values are the actual change signal
    // eslint-disable-next-line react-hooks/exhaustive-deps
    [props.valueAttribute.formatter, props.decimalPlaces]
);

⚠️ Low — E2E spec still missing test.afterEach session logout

File: packages/pluggableWidgets/slider-web/e2e/Slider.spec.js (pre-existing, in diff scope)
Problem: The file uses @mendix/run-e2e/fixtures (which may or may not bundle automatic cleanup) but has no explicit test.afterEach("Cleanup session", async ({ page }) => { await page.evaluate(() => window.mx.session.logout()); }). Per the E2E guidelines, every spec file must include this to avoid exceeding Mendix's 5-session license limit in CI.

This predates the PR, but since the file is in the diff it should be addressed here or tracked as follow-up.

Positives

• Clean root-cause fix: separating the key (locale-agnostic parseFloat(rawValue.toFixed(n))) from the label (format(rawValue)) is exactly the right model.
• Injecting format as a callback into createMarks keeps the shared utility decoupled from Mendix-specific types — editor preview works with a simple toFixed lambda, runtime uses the full NumberFormatter.
• big.js module mapper in jest.config.cjs preemptively solves the ESM-in-Jest problem cleanly.
• handleRender is now wrapped in useMemo in slider-web/Container.tsx — good performance improvement that was missing before.
• Both widget changelogs updated; widget-plugin-platform moved from devDependencies to dependencies in both widget packages (correct direction).
• Tests cover trailing-zero formatting, locale decimal separators, decimalPlaces=0, and the key-rounding alignment edge case — comprehensive for the new surface area.